Fixing that issue is part of our Grant. If we can agree on the implementation the Superoclony will handle the implementation=)

In the comment we highlighted two problems that we need to solve.

1. The identifier of the event is generated based on the name of the contract's storage.

A created issue during retreat solves that very well. The name of the events enum + the name of the variant defines a unique identifier of the event.

2. We can't collect metadata from other crates. So we can't generate metadata for those events because we don't know about them.

The approach with explicit aliases duplicates the code. But it still is better than before(you don't need to define all events in the contract's body).

    // import event as a type alias and mark as an ink(event)
    // Possibly could do this as a `use` instead or in addition.
    #[ink(event)]
    type Event0 = super::Event0;

In the comment, we suggested another approach with lazy loading of all events from the crates. This repository contains an example of how we can use the on_startup macro to collect events from different crates. It will automate the generation of metadata and simplify the usage of events. The problem with the solution is that we will include all events that were defined in sub-crates(if you import some crate you also import events from that crate into metadata). But it is not a problem at all, because it doesn't make the contract heaver. It only adds maybe not fully useful information to the metadata but now it is automated.

1. The identifier of the event is generated based on the name of the contract's storage.

#1222 during retreat solves that very well. The name of the events enum + the name of the variant defines a unique identifier of the event.

Yes that would indeed allow us to generate a useful event identifier for the contract. However it is worth noting that my current solution would also allow that since the codegen is already generating a top level Event enum from the individual specified events. e.g.

mod shared {
    #[ink::event_definition]
    pub struct SharedEvent;
}

#[ink(event)]
struct EventLocal;

#[ink(event)]
type EventB = shared::SharedEvent;

generates approximately...

enum __ink_EventBase {
    EventLocal(EventLocal),
    SharedEvent(SharedEvent),
}

So, regardless of whether we implement #1222, this solution is available to us.

2. We can't collect metadata from other crates. So we can't generate metadata for those events because we don't know about them.

The approach with explicit aliases duplicates the code. But it still is better than before(you don't need to define all events in the contract's body).

By code duplication do you mean the fact that it is required to annotate the imported shared event with #[ink(event)]?

In the comment, we suggested another approach with lazy loading of all events from the crates. This repository contains an example of how we can use the on_startup macro to collect events from different crates. It will automate the generation of metadata and simplify the usage of events. The problem with the solution is that we will include all events that were defined in sub-crates(if you import some crate you also import events from that crate into metadata).

I'm not sure about this approach, it is a big change to the metadata generation philosophy and introduces some extra complexity.

But it is not a problem at all, because it doesn't make the contract heaver. It only adds maybe not fully useful information to the metadata but now it is automated.

Currently the metadata lists all possible events defined in a contract, and with this approach the metadata would possibly contain events that are not used by the contract at all. IMO it is preferable that the metadata should be an accurate representation as to the API of the contract.

Overall the philosophy I have approached my solution is: how can we introduce shared events into the existing model of event definitions, not aiming for a complete overhaul. It should make it possible to release this as a 3.x non breaking change.

Not to say that the event codegen doesn't need some attention and thought and possibly a complete overhaul, especially optimizing codesize of topics etc.

It's just for this I want to focus on only adding shared events into the existing framework.

I guess you meant:

enum __ink_EventBase {
    EventLocal(EventLocal),
    EventB(shared::SharedEvent),
}

What benefits do we have in grouping all events into one __ink_EventBase enum? I see it helps to work with events during codegen in a more convenient way but it doesn't affect the developer anyhow. Correct me if I'm wrong=)

Do we need really combine all events into one enum if we can refactor the events collection and in that case, each event definition can be independent?

We hoped that resolving the #809 also will resolve the problem with event identifiers. So it is why #1222 looks very promising. Yes, it breaks the previous API but it allows contracts to be production-ready. Because all contracts can emit the same events(the name of the contract will not affect the identifier).

mod events {
   enum PSP22 {
       Transfer(AccountId, AccountId, Balance),
       Approve(AccountId, AccountId, Balance),
       ...
   }
}

That kind of event can be standardized and everyone can emit it.

We can extract that to a separate issue but I think all of it is related to each other and should be solved during one breaking change refactoring=)

By code duplication do you mean the fact that it is required to annotate the imported shared event with #[ink(event)]?

Yes!=)
Let's imagine that you split your contract into 4 big parts(AccessControl, PSP22, Lending, Router). Each part emits 4 events. You implemented the logic of each part in a separate crate. Also, you implemented emitting of those events in that crate.

Now you want to combine all parts together to create a final contract.

#[ink(storage)]
pub struct ComplexContract {
    access: AccessControlData,
    psp22: PSP22Data,
    lending: LendingData,
    router: RouterData,
}

impl AccessControl for ComplexContract {}
impl PSP22 for ComplexContract {}
impl Lending for ComplexContract {}
impl Router for ComplexContract {}

#[ink(event)]
type AccessControlEventChangeRole = access_contol::ChangeRole;
#[ink(event)]
type AccessControlEventRoleGranted = access_contol::RoleGranted;
#[ink(event)]
type AccessControlEventRoleRevoked = access_contol::RoleRevoked;
#[ink(event)]
type AccessControlEventCucumber = access_contol::Cucumber;

#[ink(event)]
type PSP22EventTransfer = psp22::Tranfer;
#[ink(event)]
type PSP22EventApprove = psp22::Approve;
#[ink(event)]
type PSP22EventHello = psp22::Hello;
#[ink(event)]
type PSP22EventWorld = psp22::World;

#[ink(event)]
type LendingEventBorrow = lending::Borrow;
#[ink(event)]
type LendingEventDeposit = lending::Deposit;
#[ink(event)]
type LendingEventLiquidated = lending::Liquidated;
#[ink(event)]
type LendingEventOracleChange = lending::OracleChange;

#[ink(event)]
type RouterEventCreated = router::Created;
#[ink(event)]
type RouterEventNewProposal = router::NewProposal;
#[ink(event)]
type RouterEventNewLending = router::NewLending;
#[ink(event)]
type RouterEventSwap = router::Swap;

And you scrolled here=D It is really a lot of boilerplate code and you should create a unique name for each event. Overwise it will conflict with other evens like PSP34Event::Transfer with PSP22Event::Transfer. It is better than the previous solution where you defined all events but we still can improve it=)

I'm not sure about this approach, it is a big change to the metadata generation philosophy and introduces some extra complexity.

The philosophy changed only a little bit. Instead of one place to collect metadata, we will collect it from each crate and the final metadata will be generated in the same way. In simple form: we have a static array with events. During a load of each crate, we add events to that array. When all crates are loaded it runs __ink_generate_metadata that uses that array to create the final metadata with all events. It is not hard to implement and I'm sure that it will simplify codegen=) Because we don't need to parse anything in ink_lang::contract, each ink_lang::event_definition is independent.

Currently the metadata lists all possible events defined in a contract, and with this approach, the metadata would possibly contain events that are not used by the contract at all. IMO it is preferable that the metadata should be an accurate representation as to the API of the contract.

Better to have several useless events than to forget to add some events at all=) If all events are collected automatically, it doesn't add any problem. But if we missed some events it can cause some issues.

With the introduction of upgradable contracts, we can't have accurate representation at all=) Because any contract can be upgradable or be Diamond and in that case, it is a combination of different ABIs.

Another plus for collecting all events(Example from our integration testing):
For example, you create a DEX. That means you do transfers between PSP22. Each PSP22 emits an event on the transfer. So during the execution of the Dex::swap, it will emit the known Dex::Swap event and the bunch on the unknown PSP22Event::Tranfer. With a lazy approach, collecting all events: if someone imported the trait definition of the PSP22 then he also imported the definition of the events. In that case, the indexer, UI, or another application will know how to decode PSP22Event::Tranfer. Adding more events doesn't cause any problem=)

BTW, we also can use event::PSP22::Tranfer instead of PSP22Event::Tranfer with a lazy approach and the identifier can be generated based on PSP22::Transfer=)

P. S. Sorry for long comment=D

Also, you implemented emitting of those events in that crate.

Okay I had not considered this scenario, my assumption was that the contract implementation itself would always raise the events, therefore it would have to import them either with a use or a type alias which could be decorated.

Your examples got me thinking, that shared events are tied to their corresponding traits, since they are effectively part of the contract's interface. i.e. any UI for ERCX contracts requires both standard messages and events. Indeed if you look at the standards e.g. https://eips.ethereum.org/EIPS/eip-1155 they include the event definitions. So we could have something like:

#[ink::interface]
pub mod access_control {

    #[ink(trait_definition)]
    pub trait AccessControl {
        // messages
    }
    
    #[ink(event)]
    pub enum AccessControlEvent {
        ChangeRole,
        RoleGranted,
        RoleRevoked,
        Cucumber,
    }
}

So now the trait and the events are tied together, we could even use the impl AccessControl block within the contract definition to wire up the event metadata.

The philosophy changed only a little bit. Instead of one place to collect metadata, we will collect it from each crate and the final metadata will be generated in the same way. In simple form: we have a static array with events

It is changing from explicit "pulling" to implicit "pushing" of event metadata via global state, so we need to be sure it is the correct thing to do, not just the easiest way to solve the problem.

Your examples got me thinking, that shared events are tied to their corresponding traits, since they are effectively part of the contract's interface. i.e. any UI for ERCX contracts requires both standard messages and events. Indeed if you look at the standards e.g. https://eips.ethereum.org/EIPS/eip-1155 they include the event definitions.

Yep, that idea was a reason to create that issue=)

So now the trait and the events are tied together, we could even use the impl AccessControl block within the contract definition to wire up the event metadata.

Yep, we also come up with that solution and it already fits all requirements. The trait can have some hidden __ink_events() method that returns the whole list of events(or we can generate some stuff like TraitInfo. I think we can try to experiment here), we can call it based on the impl block. And if you don't like the solution with lazy loading, then I hope we will choose that solution=)

That solution has 3 cons:

1. It makes the definition of the trait harder and introduces a new entity as #[ink_lang::interface]. Previously the definition of the trait and events was intuitive in the native Rust way. With that approach, it requires the usage of mod access_controll { ... } and some restricted constructions. It makes codegen more complex.
2. It doesn't solve the issue described in the example with Dex::swap and emitted PSP22::Transfer events. It is because we didn't pull the events that can be emitted. (Of course, it is optional problem for solving, but still a problem)
3. We can't use the same naming during the event definition. PSP22 already is used by the trait and it is not allowed to use the same name in enum PSP22 { Transfer, Approve }. We can have a separate mod events { ... } but in that case, it is a very huge definition=)

It is changing from explicit "pulling" to implicit "pushing" of event metadata via global state, so we need to be sure it is the correct thing to do, not just the easiest way to solve the problem.

You are right, it changes it in that way=)
Of course, it requires discussion and I hope we will discuss that approach. The main idea is that events are used in most cases by UI, indexers, and other aggregation stuff. ABI is used to decode the events right. It is a problem if we don't know how to decode the event. But it is not a problem if we know how to decode events that can't be emitted. It, maybe, can be a problem in the case of an indexer because the indexer will listen for events that can't be emitted by the contract. But in most cases the handler that should process the event is empty, so the indexer can not listen for events without a handler.

The main pros:

1. We don't affect the definition of the trait. The definition of the event is independent and native. The codegen is simple.
2. We can't lose any event that can be emitted, because we include all events imported into the contract.
3. The events can be defined anywhere and anyhow without any restrictions.

cons:

1. We include all events, useless and useful.

I hope that we will find and select the best approach. I'm describing only the ideas and thoughts of our team and the use cases that we faced.

Will respond properly tomorrow but regarding:

2. It doesn't solve the issue described in the example with Dex::swap and emitted PSP22::Transfer events. It is because we didn't pull the events that can be emitted. (Of course, it is optional problem for solving, but still a problem)

I did some googling about the state of that in Solidity, and looks like that is not currently supported but this PR looks like it might be a solution ethereum/solidity#10996?

I did some googling about the state of that in Solidity, and looks like that is ethereum/solidity#9765 but this PR looks like it might be a solution ethereum/solidity#10996?

The best solution is to include the event if it was somewhere emitted. But ink! is not its own language, so it is hard to identify that event was emitted. Yesterday I tried to play with rust ABI and linking attributes and with ctor but didn't find how to do that.

Superseded by #1827